Skip to content

fix: expand AD user name filter #737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
razvan merged 14 commits into from
Jul 3, 2025
Merged

fix: expand AD user name filter #737

razvan merged 14 commits into from
Jul 3, 2025

Conversation

@razvan
Copy link
Member

@razvan razvan commented Jun 25, 2025 •
edited
Loading

Description

Fixes #702

How I tested

  1. Used ad-init to set up an AD VM and a connected Kubernetes cluster.
  2. Created a new AD group called ad-test, a new AD user called razvan in the SBLE.TEST realm and added razvan to the ad-test group.
  3. Deployed the opa-operator with make run-dev
  4. Deployed the following OPA cluster (see below) and port forwarded 9476 of the OPA pod.
  5. Ran the following curl command:
❯ curl -XPOST -H "Content-type: application/json" \
-d '{"username": "[email protected]"}' \
http://localhost:9476/user | jq
{
  "id": "de639e1d-6b75-4a7d-8347-f2c8ade06d51",
  "username": "[email protected]",
  "groups": [
    "CN=Users,CN=Builtin,DC=sble,DC=test",
    "CN=Domain Users,CN=Users,DC=sble,DC=test",
    "CN=ad-test,CN=Users,DC=sble,DC=test"
  ],
  "customAttributes": {}
}

OPA cluster definition:

---
apiVersion: opa.stackable.tech/v1alpha1
kind: OpaCluster
metadata:
  name: opa
spec:
  image:
    productVersion: 1.0.1
  clusterConfig:
    userInfo:
      backend:
        experimentalActiveDirectory:
          ldapServer: sble-addc.sble.test
          baseDistinguishedName: DC=sble,DC=test
          customAttributeMappings:
            country: c
          kerberosSecretClassName: kerberos-ad
          tls:
            verification:
              server:
                caCert:
                  secretClass: tls-ad
      cache: # optional, enabled by default
        entryTimeToLive: 60s # optional, defaults to 60s
  servers:
    roleGroups:
      default:
        podOverrides:
          spec:
            containers:
              - name: bundle-builder
                imagePullPolicy: IfNotPresent
              - name: user-info-fetcher
                imagePullPolicy: IfNotPresent
                env:
                  - name: CONSOLE_LOG
                    value: DEBUG
                  - name: CONSOLE_LOG_LEVEL
                    value: DEBUG

The (debug) logs of the user-info-fetcher container showed:

+ '[' -f /stackable/kerberos/krb5.conf ']'
++ grep -oP 'default_realm = \K.*' /stackable/kerberos/krb5.conf
+ export KERBEROS_REALM=SBLE.TEST
+ KERBEROS_REALM=SBLE.TEST
+ /sbin/stackable-opa-user-info-fetcher
2025-06-26T15:31:23.126407Z  INFO stackable_opa_user_info_fetcher: Starting user-info-fetcher built_info.pkg_version="0.0.0-dev" built_info.target="x86_64-unknown-linux-gnu" built_info.bui
lt_time_utc="Tue, 1 Jan 1980 00:00:00 +0000" built_info.rustc_version="rustc 1.86.0 (05f9846f8 2025-03-31) (built from a source tarball)"
2025-06-26T15:47:19.663489Z DEBUG get_user_info{request=UserInfoRequestByName(UserInfoRequestByName { username: "[email protected]" }) ldap_server="sble-addc.sble.test"}: stackable_opa_user
_info_fetcher::backend::active_directory: requesting user from LDAP user_query_filter="(&(objectClass=user)(|([email protected]@SBLE.TEST)([email protected]
st)([email protected])))" requested_user_attrs=["objectSid", "objectGUID", "userPrincipalName", "primaryGroupID", "c"]
2025-06-26T15:47:19.664185Z DEBUG get_user_info{request=UserInfoRequestByName(UserInfoRequestByName { username: "[email protected]" }) ldap_server="sble-addc.sble.test"}: stackable_opa_user
_info_fetcher::backend::active_directory: got user from LDAP user=SearchEntry { dn: "CN=Razvan Mihai,CN=Users,DC=sble,DC=test", attrs: {"primaryGroupID": ["513"], "userPrincipalName": ["ra
[email protected]"]}, bin_attrs: {"objectGUID": [[29, 158, 99, 222, 117, 107, 125, 74, 131, 71, 242, 200, 173, 224, 109, 81]], "objectSid": [[1, 5, 0, 0, 0, 0, 0, 5, 21, 0, 0, 0, 170, 35, 214
 189, 227, 141, 132, 148, 20, 231, 109, 119, 84, 4, 0, 0[]]} }
2025-06-26T15:47:19.664293Z DEBUG get_user_info{request=UserInfoRequestByName(UserInfoRequestByName { username: "[email protected]" }) ldap_server="sble-addc.sble.test"}:user_attributes:use
r_group_distinguished_names: stackable_opa_user_info_fetcher::backend::active_directory: computed primary group SID for user user_sid=S-1-5-21-3184927658-2491715043-2003691284-1108 primary
_group_sid=S-1-5-21-3184927658-2491715043-2003691284-513 primary_group_relative_id=513
2025-06-26T15:47:19.664323Z DEBUG get_user_info{request=UserInfoRequestByName(UserInfoRequestByName { username: "[email protected]" }) ldap_server="sble-addc.sble.test"}:user_attributes:use
r_group_distinguished_names: stackable_opa_user_info_fetcher::backend::active_directory: requesting user groups from LDAP groups_query_filter="(&(objectClass=group)(|(objectSid=S-1-5-21-31
84927658-2491715043-2003691284-513)(member:1.2.840.113556.1.4.1941:=<SID=S-1-5-21-3184927658-2491715043-2003691284-513>)(member:1.2.840.113556.1.4.1941:=<SID=S-1-5-21-3184927658-2491715043
-2003691284-1108>)))" requested_group_attrs=["dn"]

where the LDAP search filter was:

(&(objectClass=user)(|([email protected]@SBLE.TEST)([email protected]
st)([email protected])))

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

  • Changes are OpenShift compatible
  • CRD changes approved
  • CRD documentation for all fields, following the style guide.
  • Helm chart can be installed and deployed operator works
  • Integration tests passed (for non trivial changes)
  • Changes need to be "offline" compatible
  • Links to generated (nightly) docs added
  • Release note snippet added

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • (Integration-)Test cases added
  • Documentation added or updated. Follows the style guide.
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Feature Tracker has been updated
  • Proper release label has been added
  • Links to generated (nightly) docs added
  • Release note snippet added
  • Add type/deprecation label & add to the deprecation schedule
  • Add type/experimental label & add to the experimental features tracker

@razvan razvan self-assigned this Jun 25, 2025
@nightkr nightkr mentioned this pull request Jun 26, 2025
Closed
nightkr
nightkr reviewed Jun 26, 2025
View reviewed changes
@razvan razvan marked this pull request as ready for review June 27, 2025 07:46
@razvan
Copy link
Member Author

razvan commented Jun 30, 2025

Depends on stackabletech/krb5-rs#1

nightkr
nightkr reviewed Jul 1, 2025
View reviewed changes
Copy link
Contributor

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality-wise this seems to work fine for me. Do we want to add cases for this to the ad-user-info test?

@razvan razvan mentioned this pull request Jul 2, 2025
Merged
@razvan
Copy link
Member Author

razvan commented Jul 2, 2025

Functionality-wise this seems to work fine for me. Do we want to add cases for this to the ad-user-info test?

Indeed. Successfully tested with the help of stackabletech/ad-init#8

nightkr
nightkr reviewed Jul 2, 2025
View reviewed changes
@razvan razvan requested a review from nightkr July 2, 2025 13:38
nightkr
nightkr reviewed Jul 2, 2025
View reviewed changes
@razvan razvan requested a review from nightkr July 3, 2025 10:54
nightkr
nightkr approved these changes Jul 3, 2025
View reviewed changes
Copy link
Contributor

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if tests pass

@razvan
Copy link
Member Author

razvan commented Jul 3, 2025 

❯ ./scripts/run-tests --skip-release --skip-delete --test ad-user-info
...
--- PASS: kuttl (17.51s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/ad-user-info_opa-latest-1.4.2_openshift-false (17.21s)
PASS

@razvan razvan added this pull request to the merge queue Jul 3, 2025
Merged via the queue into main with commit 399bd76 Jul 3, 2025
17 checks passed
@razvan razvan deleted the fix/issue-702 branch July 3, 2025 12:26
dervoeti pushed a commit that referenced this pull request Oct 29, 2025
@razvan @dervoeti
fix: expand AD user name filter (#737)
6f2a7fa 
* fix: expand AD user name filter

* fix rustfmt

* fix ldap query syntax

* extract and use KERBEROS_REALM env var

* update changelog

* use krb5-rs to read realm name

* add todo comment

* review feedback

* expand ad-user-info test

* also search for upn without realm

* update cargo deps

* update test

* test for normalized user name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nightkr nightkr nightkr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@razvan razvan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

User Info Fetcher: Retrieve user info by sAMAccountName

3 participants

@razvan @nightkr